-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix timeline initial date range regression #1162
Conversation
When refactoring into TimelineContext, there was a regression regarding the date range that is initially loaded. e-mission#1142 (comment) It used to load 7 days before the pipelineEnd up to today. After the change, it would load 7 days before today up to today. If the user has travel everyday this ends up being equivalent. But if there isn't recent travel, this becomes a problem. So in TimelineContext, the initial dateRange needs to depend on pipelineRange. instead of giving an initial value upfront, we'll let dateRange be null until pipelineRange is set, at which point we'll also set dateRange. This means we have to consider some extra cases where dateRange can be null.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1162 +/- ##
==========================================
- Coverage 30.59% 30.57% -0.03%
==========================================
Files 118 118
Lines 5216 5220 +4
Branches 1154 1107 -47
==========================================
Hits 1596 1596
- Misses 3618 3622 +4
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I am going to merge this for now, but I think it would be good to think through what will happen if the network is down and we are not able to retrieve the pipelineRange. Not sure what it does right now, but at some point it used to be able to show draft trips. with this change, I think it will not show anything. |
After this fix, things look at lot better. The regressions from #1142 seem to be fixed, there are no errors with aggregate trips. I still don't see the group bar and now I don't see the "worst case". The lack of group bar may just because we don't have any other data for that week, but I don't understand why the worst case is not showing up. @JGreenlee last fix (if needed!) and then we are ready to move this to production!
|
I checked this on my personal phone, and I see the worst case, but not the group average. I also got a timeout on retrieving the aggregate value the first time I went to the dashboard. @JGreenlee |
@JGreenlee example of dashboard (intermittently) not updating
|
My personal phone is on nrel_commute, so I also upgraded it and then checked the logs. We do get the correct
|
When refactoring into TimelineContext, there was a regression regarding the date range that is initially loaded. #1142 (comment)
It used to load 7 days before the pipelineEnd up to today. After the change, it would load 7 days before today up to today. If the user has travel everyday this ends up being equivalent. But if there isn't recent travel, this becomes a problem.
So in TimelineContext, the initial dateRange needs to depend on pipelineRange. instead of giving an initial value upfront, we'll let dateRange be null until pipelineRange is set, at which point we'll also set dateRange. This means we have to consider some extra cases where dateRange can be null.